refactor(NODE-6616): short circuit EJSON stringifying #4360
Conversation
nbbeeken
changed the title
nbbeeken
force-pushed
the
NODE-6616-ejson-max-len
branch
from
4bc5283 to
9e2721f
Compare
nbbeeken
force-pushed
the
NODE-6616-ejson-max-len
branch
from
9e2721f to
0e83e17
Compare
| if (typeof value === 'string') { | ||
| currentLength += value.length; | ||
| } else if (typeof value === 'number' || typeof value === 'bigint') { | ||
| currentLength += 20; |
There was a problem hiding this comment.
where is this lengths coming from? wouldn't the length of numbers be dependent on the actual number being stringified (ex: 0 would be 1, but 1.1234 would be 6?)
There was a problem hiding this comment.
Yes correct they would depend on the value. I can improve this to calculate the number of digits from the value.
Generally, the lengths here are approximations when we can't determine the length without actually stringifying the value in an attempt to avoid paying that potentially expensive cost. With a better implementation, we should always get a bit over maxLen returned to us and then slicing ensures we actually hit the target.
nbbeeken
force-pushed
the
NODE-6616-ejson-max-len
branch
from
0e83e17 to
d588ada
Compare
| let currentLength = 0; | ||
| const maxDocumentLengthEnsurer = function maxDocumentLengthEnsurer(key: string, value: any) { | ||
| if (currentLength >= maxDocumentLength) { | ||
| return undefined; |
There was a problem hiding this comment.
This will essentially redact the keys entirely, right? I guess you've considered the option of using a placeholder and decided against it?
There was a problem hiding this comment.
I didn't consider a placeholder, that may work just as well for performance too since it would be a hardcoded value (maybe an empty string?). True, it will redact keys but the thinking was that those would be lost anyway by the slicing that follows.
Description
What is changing?
Is there new documentation needed for these changes?
What is the motivation for this change?
Release Highlight
Fill in title or leave empty for no highlight
Double check the following
npm run check:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript